10-6 [BE] [검색리스트] pagination 범위 초과시 error 처리 #63

leesungbin · 2022-11-24T03:18:14Z

개요

특정 키워드에 대한 totalPages가 20일 경우, 사용자가 21 page에 대한 요청을 보내면 NotFoundException(404)를 던져줍니다.

작업사항

page > totalPages 일 경우, NotFoundException(404) 전달
citations 내림차순 순으로 데이터 전달
title 이 항상 있는 데이터만 전달
axios timeout 20,000ms 으로 설정
- timeout시 RequestTimeoutException(408) 전달
crossref etiquette 설정 (MAIL_TO 환경변수 추가 필요)

추가

searchController test 코드 수정

 PASS  src/search/tests/search.controller.spec.ts (7.605 s)
  SearchController
    /search/auto-complete
      ✓ getAutoCompletePapers - keyword=coffee 일 때 PaperInfo[]를 return (777 ms)
      ✓ keyword 미포함시 error - GET /search/auto-complete?keyword= (18 ms)
    /search
      ✓ getPapers - keyword='coffee' 일 때 PaperInfoExtended[]를 return (12 ms)
      ✓ keyword 미포함시 error - GET /search?keyword= (7 ms)
      ✓ rows<=0 이거나, rows 값이 integer가 아닐 경우 error - GET /search?keyword=coffee&rows=<rows> (7 ms)
      ✓ page<=0 이거나, page 값이 integer가 아닐 경우 error - GET /search?keyword=coffee&page=<page> (6 ms)
      ✓ page>max 이면 error - GET /search?keyword=coffee&page=<page> (5 ms)
    /search/paper
      ✓ getPaper - doi=10.1234/some_doi 일 때 PaperInfoDetail을 return (4 ms)
      ✓ doi가 입력되지 않을 경우 error - GET /search/paper?doi= (6 ms)

Test Suites: 1 passed, 1 total
Tests:       9 passed, 9 total
Snapshots:   0 total
Time:        7.823 s, estimated 8 s

/search/paper 의 결과로 오는 referenceList의 각 값에 key 추가

리뷰 요청사항

src/search/tests/search.service.mock.ts 에서 mockup 하는 방식
src/search/tests/search.controller.spec.ts 에서 데이터의 type을 검사하는 방식
```
const c = class T implements PaperInfo {};
Object.entries(item).forEach(([key, value]) => {
  try {
    c[key] = value;
  } catch (err) {
    throw err;
  }
});
```
interface에 맞는 class를 생성해서 item의 key, value가 해당 class에 선언될 수 있는지 여부를 확인하는 방식인데, class implements에는 generic을 사용할 수가 없어서,, 그 때 그 때 함수를 선언해주어야하네요.
이 방식이 아니면, PaperInfo, PaperInfoExtended, PaperInfoDetail interface들을 이전처럼 class로 돌려서 generic 함수를 만드는 방법을 사용할 수는 있을 것 같습니다.
어떤 방식으로 개선할 수 있을까요?
-> 이 부분은 swagger api를 붙이기 위한 편의성을 위해 class로 선언하겠습니다.

leesungbin · 2022-11-24T03:21:58Z

#44

sujinleeme · 2022-11-27T16:39:57Z

backend/src/search/search.controller.ts

    this.rankingService.insertRedis(keyword);
+


테스트도 추가해주시면 좋겠네요.

mockup 방식 변경

yeynii

정말 꼼꼼하게 테스트 코드를 작성해주신것 같아요. 외부 모듈 mock 작업도 여러모로 신경쓰신게 느껴졌어요. 존경합니다👍

yeynii · 2022-11-28T18:35:00Z

backend/src/search/tests/search.controller.spec.ts

 describe('SearchController', () => {
  let controller: SearchController;
+  let app: INestApplication;


Suggested change

let app: INestApplication;

let service: SearchService;

let app: INestApplication;

상단에 service를 선언해서 각 테스트케이스들이 service 내부 메서드들을 테스트에 활용할 수 있게하면 좋을 것 같습니다.

yeynii · 2022-11-28T18:37:12Z

backend/src/search/tests/search.controller.spec.ts

+        {
+          provide: ElasticsearchService,
+          useValue: elasticService,
+        },
      ],
    }).compile();
    controller = module.get(SearchController);


Suggested change

controller = module.get(SearchController);

controller = module.get(SearchController);

service = module.get(SearchService);

그러면 service module의 정의부가 필요해집니다.

yeynii · 2022-11-28T18:59:13Z

backend/src/search/tests/search.controller.spec.ts

+      // Case 1. elasticsearch에 data가 없을 경우
+      const keyword = 'coffee';


Suggested change

// Case 1. elasticsearch에 data가 없을 경우

const keyword = 'coffee';

const spyGetElasticSearch = jest.spyOn(service, 'getElasticSearch');

const spyGetCrossRefData = jest.spyOn(service, 'getCrossRefData');

const keyword = 'coffee';

// Case 1. elasticsearch에 data가 없을 경우

jest.spyOn을 사용하면 service의 내부 함수를 mock으로 대체하지 않아도 호출여부, 호출 방식 등을 알아낼 수 있다고 합니다.
두가지 케이스에서 searchService.getElasticSearch, searchService.getCrossRefData 의 호출 여부를 테스트에 활용하면 분리된 케이스 검증이 가능할 것 같아요.
그리고 keyword 변수는 두 케이스에서 공통으로 사용하므로 같이 주석 상단으로 옮기면 좋을 것 같습니다.

오~ 그렇네요. 서비스에 spyOn을 붙여서 함수가 잘 호출되고 있는지 확인하도록 할게요.

yeynii · 2022-11-28T19:13:53Z

backend/src/search/tests/search.controller.spec.ts

+      // Case 2. elasticsearch에 data가 있는 경우
+      const itemsByElasticsearch = await controller.getAutoCompletePapers({ keyword });
+      expect(itemsByElasticsearch.length).toBe(5);
+      itemsByElasticsearch.forEach((item) => {
+        expect(item instanceof PaperInfo).toBe(true);
+      });
+    });
+    it('keyword 미포함시 error - GET /search/auto-complete?keyword=', () => {
+      const url = (keyword: string) => `/search/auto-complete?keyword=${keyword}`;
+      return request(app.getHttpServer()).get(url('')).expect(400);
+    });


mockElasticService() 함수에서 mockResolvedValueOnce로 elasticService.search의 목업 케이스 분리를 잘 해주셔서, 실제 테스트 코드내에서도 두 케이스가 다르게 동작하고 있는것을 확인했습니다.
그런데 현재의 코드는 두 케이스의 검증로직이 동일해서, 두 케이스가 실제로 구분되어 실행되고 있는지 확인하는 로직도 있으면 더 좋을 것 같아요.

Suggested change

// Case 2. elasticsearch에 data가 있는 경우

const itemsByElasticsearch = await controller.getAutoCompletePapers({ keyword });

expect(itemsByElasticsearch.length).toBe(5);

itemsByElasticsearch.forEach((item) => {

expect(item instanceof PaperInfo).toBe(true);

});

});

it('keyword 미포함시 error - GET /search/auto-complete?keyword=', () => {

const url = (keyword: string) => `/search/auto-complete?keyword=${keyword}`;

return request(app.getHttpServer()).get(url('')).expect(400);

});

expect(spyGetElasticSearch).toBeCalledTimes(1);

expect(spyGetCrossRefData).toBeCalledTimes(1);

// Case 2. elasticsearch에 data가 있는 경우

const itemsByElasticsearch = await controller.getAutoCompletePapers({ keyword });

expect(itemsByElasticsearch.length).toBe(5);

itemsByElasticsearch.forEach((item) => {

expect(item instanceof PaperInfo).toBe(true);

});

expect(spyGetElasticSearch).toBeCalledTimes(2);

expect(spyGetCrossRefData).toBeCalledTimes(1);

case1: spyGetElasticSearch, spyGetCrossRefData 모두 호출
case2: spyGetElasticSearch만 호출

이렇게 검증하면 되지 않을까요?

yeynii · 2022-11-28T19:30:38Z

backend/src/search/tests/search.service.mock.ts

+  });
+  search.mockResolvedValue({


mockResolvedValueOnce로 호출 횟수에따라 케이스를 구분해서 테스트에 활용할 수 있었군요! 좋은 방법을 찾아서 적용하신 것 같아요. 배워갑니다.

Suggested change

});

search.mockResolvedValue({

})

.mockResolvedValue({

이부분 체이닝이 가능하더라고요. jest 공식 docs 예제에서도 저 메서드를 사용할 때 체이닝 패턴을 주로 사용 하던데, 개인적으로는 체이닝을 하면 이런 순서로 호출될것이다~ 라는 느낌을 주는 것 같습니다..!?!! (물론 순서가 바뀌더라도 mockResolvedValueOnce가 항상 먼저 불리긴합니다..) 사소한부분이라... 성빈님의 취향에 맡기겠습니다.

체이닝 되는 게 가독성이 더 좋은 것 같습니다!

yeynii

확인했습니다.

Palwol

테스트코드를 정말 꼼꼼하게 작성해 주셨네요 👍 테스트 이름도 알기 쉽게 작성되어 있어서 이해가 쉬웠습니다.

Palwol · 2022-11-29T12:13:45Z

backend/src/search/tests/search.controller.spec.ts

+    it('rows<=0 이거나, rows 값이 integer가 아닐 경우 error - GET /search?keyword=coffee&rows=<rows>', () => {
+      const url = (rows: string | number) => `/search?keyword=${keyword}&rows=${rows}`;
+      const rowsNotAvailables = [-1, -0.1, '0', 'value'];
+      rowsNotAvailables.forEach((value) => {
+        request(app.getHttpServer()).get(url(value)).expect(400);
+      });
+    });


rows도 crossref api에서 한 번에 불러올 수 있는 갯수로 최댓값을 정해두면 좋을 것 같아요.
최댓값 이상을 요청하면 최댓값으로 돌려준다거나 하는 식으로 예외처리 해볼 수 있을 것 같습니다.

Palwol · 2022-11-29T12:22:45Z

backend/src/search/tests/search.controller.spec.ts

+  describe('/search/paper', () => {
+    it(`getPaper - doi=10.1234/some_doi 일 때 PaperInfoDetail을 return`, async () => {
+      const doi = '10.1234/some_doi';
+      const paper = await controller.getPaper({ doi });
+      expect(paper.references).toBe(5);
+      expect(paper.referenceList.length).toBe(5);
+      expect(paper).toBeInstanceOf(PaperInfoDetail);
+    });
+    it('doi가 입력되지 않을 경우 error - GET /search/paper?doi=', () => {
+      const url = (keyword: string) => `/search/paper?doi=${keyword}`;
+      request(app.getHttpServer()).get(url('')).expect(400);
    });


입력받은 doi가 올바르지 않을 때(crossref에 없을 때)의 케이스도 있으면 좋을 것 같습니다! (404 에러)

leesungbin added 4 commits November 24, 2022 11:40

feat: crossref api timeout(20000), mailto 에티켓 설정

8caca0f

feat: title이 있는 논문만 목록으로 가져온다

f6e76b5

refactor: autoComplete data 검색 로직 refactor, page > totalPage 일 때 error

61e000d

feat: page > totalPages 일 경우 NotFoundException

3c679ea

leesungbin marked this pull request as ready for review November 24, 2022 03:21

github-actions bot requested review from Palwol and yeynii November 24, 2022 03:21

leesungbin and others added 3 commits November 24, 2022 13:26

Merge branch 'dev' into feature/crossref-etiquette

1330a70

fix: 함수 인자 순서 변경으로 인한 conflict 해결

5ddc42d

fix: totalItems 포함

e157aa0

sujinleeme reviewed Nov 27, 2022

View reviewed changes

leesungbin closed this Nov 28, 2022

leesungbin reopened this Nov 28, 2022

fix: solve merge conflict

d906570

leesungbin linked an issue Nov 28, 2022 that may be closed by this pull request

[BE] 논문 상세정보 api 호출 시 reference가 없으면 500 에러가 발생하는 오류 #75

Closed

leesungbin added 4 commits November 28, 2022 19:07

fix: doi -> DOI 변경

71f9bdc

test: test 코드 refactor

2a79f32

mockup 방식 변경

feat: referenceList에 key 추가

86bc208

test: search controller test 로직 개선

c89fdaa

yeynii reviewed Nov 28, 2022

View reviewed changes

leesungbin added 2 commits November 29, 2022 09:36

test: add spy on search service

81ababa

test: page>max 인 case 추가

e595e92

Palwol mentioned this pull request Nov 29, 2022

13-4 [FE] [논문 상세 - 논문 정보] 검색한 논문의 제목, 저자, DOI, 인용논문 목록을 보여주고, DOI 클릭 시 논문 원본으로 이동한다. #76

Merged

yeynii approved these changes Nov 29, 2022

View reviewed changes

Palwol approved these changes Nov 29, 2022

View reviewed changes

leesungbin merged commit 61a6d80 into dev Nov 29, 2022

leesungbin deleted the feature/crossref-etiquette branch December 1, 2022 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

10-6 [BE] [검색리스트] pagination 범위 초과시 error 처리 #63

10-6 [BE] [검색리스트] pagination 범위 초과시 error 처리 #63

leesungbin commented Nov 24, 2022 •

edited

Loading

leesungbin commented Nov 24, 2022

sujinleeme Nov 27, 2022

yeynii left a comment

yeynii Nov 28, 2022

yeynii Nov 28, 2022

yeynii Nov 28, 2022

leesungbin Nov 29, 2022

yeynii Nov 28, 2022

yeynii Nov 28, 2022

leesungbin Nov 29, 2022

yeynii left a comment

Palwol left a comment

Palwol Nov 29, 2022

Palwol Nov 29, 2022

	let app: INestApplication;
	let service: SearchService;
	let app: INestApplication;

	controller = module.get(SearchController);
	controller = module.get(SearchController);
	service = module.get(SearchService);

		// Case 1. elasticsearch에 data가 없을 경우
		const keyword = 'coffee';

10-6 [BE] [검색리스트] pagination 범위 초과시 error 처리 #63

10-6 [BE] [검색리스트] pagination 범위 초과시 error 처리 #63

Conversation

leesungbin commented Nov 24, 2022 • edited Loading

개요

작업사항

추가

리뷰 요청사항

leesungbin commented Nov 24, 2022

Choose a reason for hiding this comment

yeynii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeynii left a comment

Choose a reason for hiding this comment

Palwol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leesungbin commented Nov 24, 2022 •

edited

Loading